Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use portable-atomic instead of atomic-polyfill. #328

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Dec 23, 2022

I intend to deprecate atomic-polyfill soon, in favor of portable-atomic. It supports more archs and is more actively maintained. The only thing it can't do yet is use critical-section, but it should soon it already can: taiki-e/portable-atomic#51

@Dirbaio Dirbaio force-pushed the portable-atomic branch 2 times, most recently from 39bf5cc to 1001759 Compare December 23, 2022 18:31
@korken89
Copy link
Contributor

Hi please rebase to fix tsan failure

@korken89
Copy link
Contributor

Hi, I gave this a check and saw something weird.

RUSTFLAGS="--cfg=portable_atomic_unsafe_assume_single_core"

Is there a reason this is not a feature to heapless?

@Dirbaio
Copy link
Member Author

Dirbaio commented Jan 13, 2023

it's a feature for atomic-polyfill. See its readme. The reasoning for making it a --cfg and not a Cargo feature is that enabling it on non-multicore chips can cause unsoundness, so the author wants to enforce only the final binary crate can enable it, not library crates. (it's a decision I don't agree with, see some discussion here... 🤷 )

@korken89
Copy link
Contributor

Hmm, I see - that was one long discussion. 😅

Not sure what I think of it yet, I'll leave this open until I can come to terms with --cfg or if @japaric forces it through.

@Dirbaio Dirbaio force-pushed the portable-atomic branch 3 times, most recently from e3ac654 to 13a9d1a Compare March 14, 2023 18:07
@Dirbaio
Copy link
Member Author

Dirbaio commented Mar 14, 2023

Pushed new version:

  • Rebase
  • Use portable-atomic 1.0
  • Use portable-atomic/critical-section feature in CI tests, instead of the --cfg.

With the current implementation, for targets without native CAS, the end-user will have to add a dep on portable-atomic to configure it. Either enabling the critical-section feature, or the evil --cfg=portable_atomic_unsafe_assume_single_core. Not sure if that's OK, or how we can do better.

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines +5 to +11
//! # Portability
//!
//! This module requires CAS atomic instructions which are not available on all architectures
//! (e.g. ARMv6-M (`thumbv6m-none-eabi`) and MSP430 (`msp430-none-elf`)). These atomics can be
//! emulated however with [`portable-atomic`](https://crates.io/crates/portable-atomic), which is
//! enabled with the `cas` feature and is enabled by default for `thumbv6m-none-eabi` and `riscv32`
//! targets.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this SPSC implementation does not require atomic CAS and only uses atomic load/store.

(FWIW, portable-atomic provides multicore-safe atomic load/store for riscv without A extension, even without critical-section feature or unsafe single-core cfg)

Cargo.toml Outdated Show resolved Hide resolved

[target.'cfg(target_arch = "avr")'.dependencies]
atomic-polyfill = { version = "1.0.1", optional = true }
portable-atomic = { version = "1.0", optional = true }
Copy link

@taiki-e taiki-e Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this time, a builtin AVR target (avr-unknown-gnu-atmega328) does not provide atomic, and AVR targets used in avr-hal do not provide atomic load-store of pointer width. So I guess spsc which uses AtomicUsize will always require portable-atomic.

However, LLVM can provide 16-bit atomic load/store for AVR, so there is no need to necessarily remove optional=true here. (I should send a patch to rustc/avr-hal to update the target specs. EDIT: opened rust-lang/rust#114495)

@taiki-e
Copy link

taiki-e commented Mar 24, 2023

With the current implementation, for targets without native CAS, the end-user will have to add a dep on portable-atomic to configure it. Either enabling the critical-section feature, or the evil --cfg=portable_atomic_unsafe_assume_single_core. Not sure if that's OK, or how we can do better.

When using the critical-section feature, I think what the user needs to do is the same as when using any other crates that have critical-section feature, such as once_cell.
So I'm not sure if that is the best, but I think it is fine.

In any case, I hope examples I just added clarifies the step to use them...

(Regarding whether to use cfg or feature, I'm considering adding a comment something like "If you feel that using the single-core cfg is annoying, consider using the critical-section feature"...)

@Dirbaio
Copy link
Member Author

Dirbaio commented Mar 24, 2023

@taiki-e: There's many cases where a crate knows it's going to run in single-core (for example embassy-stm32 with stm32f4xx feature. All F4 chips are single-core). In these cases it'd want to enable the atomic-polyfill/unsafe-assume-single-core Cargo feature if it existed, so that it all Just Works for the end user.

Asking the user to add nonstandard --cfg args is very annoying. Using the critical-section feature to avoid it is suboptimal, it'll result in worse performance unnecessarily (it has to acquire the CS for load/store, while unsafe-assume-single-core only has to for CAS ops).

Could you please consider adding a unsafe-assume-single-core Cargo feature?

@taiki-e
Copy link

taiki-e commented Mar 28, 2023

unsafe-assume-single-core Cargo feature

After discussion in tokio-rs/bytes#573, I'm starting to think that adding per-target/implementation unsafe-assume-single-core-* features (e.g., unsafe-assume-single-core-cortex-m) might be fine.

The cross-platform unsafe-assume-single-core feature can be roughly enabled by the user, and also cargo v1 resolver may enable it for unintended targets, so I'm not positive to add it.

However, I think per-target/implementation unsafe-assume-single-core-* features that cause compile error on unsupported targets might be enough (though not perfect1) to prevent such an accidental misuse (cross-platform unsafe-assume-single-core feature silently causes problems). (I have previously seen a GUI-related crate that causes a compile error and suggests the use of cargo v2 resolver, when the feature is accidentally enabled on unsupported targets.)

Footnotes

  1. It does not address all of my original concerns, but I think it can address the most annoying part " even if the library maintainer understands the target correctly and takes the correct action, (and the end user takes the correct action) it may not work correctly for the end user".

@Dirbaio
Copy link
Member Author

Dirbaio commented Jul 11, 2023

ping @korken89

atomic-polyfill is now deprecated, and portable-atomic 1.4 added the unsafe-assume-single-core Cargo feature so now there's no case where you need --cfgs in embedded usage.

@Dirbaio Dirbaio added this pull request to the merge queue Oct 30, 2023
Merged via the queue into rust-embedded:main with commit 7e6bff3 Oct 30, 2023
69 checks passed
@Dirbaio Dirbaio deleted the portable-atomic branch October 30, 2023 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants